-
-
Notifications
You must be signed in to change notification settings - Fork 358
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ask "Is this inside a building" #5248
Conversation
app/src/main/java/de/westnordost/streetcomplete/quests/amenity_indoor/AddIsAmenityIndoor.kt
Show resolved
Hide resolved
Sure, I will |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, definitely delete AddIsDefibrillatorIndoor, please. We can fix the documentation afterwards.
Also, since you replace/rename the quest, you should add an alias to typeAliases
, i.e.
"AddIsDefibrillatorIndoor" to AddIsAmenityIndoor...
app/src/main/java/de/westnordost/streetcomplete/quests/amenity_indoor/AddIsAmenityIndoor.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/de/westnordost/streetcomplete/quests/amenity_indoor/AddIsAmenityIndoor.kt
Show resolved
Hide resolved
app/src/main/java/de/westnordost/streetcomplete/quests/amenity_indoor/AddIsAmenityIndoor.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another review, I found one bug and one "lint" error
app/src/main/java/de/westnordost/streetcomplete/quests/amenity_indoor/AddIsAmenityIndoor.kt
Outdated
Show resolved
Hide resolved
} | ||
|
||
override fun isApplicableTo(element: Element) = | ||
nodesFilter.matches(element) && hasAnyName(element.tags) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isApplicableTo
is called also offline for any element that is updated due to the user changing it.
So for example when you solve the building quest, the app subsequently checks with this function whether for the new version of the building any new quests apply. In this case, the building levels quest will return true here, so a new quest is created.
Now, this means for your implementation here that if any e.g. amenity=atm
is updated through any other quest (e.g. checking whether it is still there), the method here will always return true, ie.e. the AddIsAmenityIndoor
quest is created. We do not want that.
In fact, as long as we do not have any information about the surrounding buildings, we cannot answer whether it applicable or not. Or well, if the given element does not match the nodesFilter
or does not have any name, then we can say that it is not applicable, but otherwise, we can't tell. In this case, we want to return null
.
Also have a look at for example the housenumber quest as it also depends on the surrounding building geometry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, i'm not sure if i understand this right.
Would it just be
override fun isApplicableTo(element: Element) { if (!nodesFilter.matches(element) || !hasAnyName(element.tags)) false else null }
or should i think of a more complex filtering?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this function, yes. However, you use this function also in getApplicableElements
, so there you should probably inline nodesFilter.matches(element) && hasAnyName(element.tags)
nodesPositions.insert(node.position) | ||
} | ||
|
||
buildings.removeAll { building -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A further performance improvement would be possible here.
Currently for every node, you check if it is contained within any building that is near one of these nodes.
You could also reduce the nodes
beforehand that are not near any building. (add all nodes near buildings to a set and then only iterate through these instead of all nodes
)
However, probably not worth it because I guess there are not that many "small amenity nodes" looking at the filter you wrote, so not required for this PR to be merged. Just wanted to note this.
app/src/main/java/de/westnordost/streetcomplete/quests/amenity_indoor/AddIsAmenityIndoor.kt
Show resolved
Hide resolved
Something went wrong with your merge, look at the changes on github now. |
Yes,but i don't really know, what went wrong. |
Ok, sorry for the botched merge, i now created a new pull request |
Related Issue: #5175
I had no idea for a good quest icon, can someone else take care on this?
This quest should replace "AddIsDefibrillatorInside", but i noticed that the defibrillator quest is used in CONTRIBUTING_A_NEW_QUEST.md for explanation, so we should either adapt this documentation or keep the AddIsDefibrillatorIndoor file